Add MCP - #689
Conversation
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a repository-hosted Copilot plugin + MCP server/CLI integration for QDK Chemistry, including workspace binding enforcement, plugin install/update/rebind automation, and extensive tests/docs. It also updates some core chemistry defaults/serialization behaviors to better support MCP-driven workflows.
Changes:
- Introduce an MCP server entry point (
qdkchemmcp), workspace binding middleware, and a CLI plugin installer workflow for deploying Copilot agents/skills + MCP configs into a workspace. - Add Copilot plugin assets (agents, skills, marketplace metadata) and documentation describing agent-driven workflows.
- Update nuclear-derivative defaults (analytic gradients + Hessian support) and Orbitals/HDF5 serialization to preserve empty active-space partitions.
Reviewed changes
Copilot reviewed 60 out of 63 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents Copilot plugin installation and workspace binding expectations. |
| python/tests/ui/test_workspace_binding.py | Tests workspace binding and middleware enforcement. |
| python/tests/ui/test_plugin_installer.py | Tests plugin install/update/rebind flows and workspace deployment. |
| python/tests/ui/test_mcp_tools.py | Validates MCP tools behavior, remote execution timing, conversions, cataloging. |
| python/tests/ui/test_mcp_resource_estimator.py | Tests QRE-backed MCP resource estimation tool shape/behavior. |
| python/tests/ui/test_mcp_catalog.py | Tests MCP tool description compaction and catalog invariants. |
| python/tests/ui/test_agent_plugin.py | Validates plugin file completeness and MCP env constraints. |
| python/tests/ui/init.py | Declares ui test package. |
| python/tests/test_nuclear_derivative.py | Updates expected default derivative implementation + geometry optimizer registration. |
| python/tests/test_circuit_executor_data.py | Adds content-hash error-path coverage for unsupported metadata. |
| python/tests/test_cache.py | Adds duplicate cache registration error coverage. |
| python/src/qdk_chemistry/ui/workspace.py | Implements workspace binding tool + middleware gate for MCP processes. |
| python/src/qdk_chemistry/ui/validation.py | Adds filename marker validation + project validation decorator. |
| python/src/qdk_chemistry/ui/plugin_installer.py | Implements Copilot plugin installation and venv-bound MCP command rebinding. |
| python/src/qdk_chemistry/ui/mcp.py | Adds MCP server CLI with compact tool descriptions and compatibility flags. |
| python/src/qdk_chemistry/ui/io.py | Adds generic file I/O helpers for JSON/HDF5 tool paths. |
| python/src/qdk_chemistry/ui/config.py | Adds MCP server configuration with scratch/projects/cache/jobs directories. |
| python/src/qdk_chemistry/ui/init.py | Lazy-loads UI submodules to reduce import-time side effects. |
| python/src/pybind11/algorithms/nuclear_derivative.cpp | Exposes derivative hash and updates QDK derivative docs/behavior. |
| python/src/pybind11/algorithms/factory_bindings.hpp | Adds helper templates for remote execution proxying and run hashing. |
| python/pyproject.toml | Adds MCP/argcomplete deps and new CLI scripts. |
| docs/source/user/agents.rst | New user docs for agent/MCP workflows. |
| docs/source/index.rst | Adds agents doc page to TOC. |
| cpp/tests/test_orbitals_comprehensive.cpp | Ensures empty active space with inactive orbitals remains a defined partition. |
| cpp/tests/test_nuclear_derivative.cpp | Updates derivative defaults + adds analytic Hessian behavior tests. |
| cpp/src/qdk/chemistry/data/orbitals.cpp | Treats inactive-space presence as “has_active_space” for partition semantics. |
| cpp/src/qdk/chemistry/data/hdf5_serialization.cpp | Writes/reads empty vectors safely for round-trip serialization. |
| cpp/src/qdk/chemistry/algorithms/qdk_nuclear_derivative.hpp | Adds QDK-specific derivative settings (finite-difference step for Hessian). |
| cpp/src/qdk/chemistry/algorithms/qdk_nuclear_derivative.cpp | Implements Hessian via finite differences of analytic gradients. |
| cpp/include/qdk/chemistry/algorithms/nuclear_derivative.hpp | Updates docs + makes qdk the default derivative implementation. |
| cpp/include/qdk/chemistry/algorithms/geometry_optimization.hpp | Defaults geometry optimization derivative calculator to qdk. |
| copilot-plugins/qdk-chemistry/skills/remote-execution/SKILL.md | Adds remote-execution skill content. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-overview/SKILL.md | Adds overview skill content. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-overview/references/workflow-patterns.md | Adds workflow routing reference. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-overview/references/playbook-index.md | Adds playbook index. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-overview/references/how-qdk-chemistry-works.md | Adds factory/tooling conventions reference. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-mcp/SKILL.md | Adds MCP skill content and policies. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-mcp/references/things-that-go-wrong.md | Adds failure-modes reference. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-mcp/references/quantum-resource-compression.md | Adds resource-compression reference. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-mcp/references/qpe-and-state-prep.md | Adds QPE/state-prep reference. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-mcp/references/active-space-guide.md | Adds active-space selection reference. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-coding/SKILL.md | Adds Python SDK coding skill content. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-coding/references/python-sdk-reference.md | Adds Python SDK reference. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-coding/references/example-n2-stretched.md | Adds worked example reference. |
| copilot-plugins/qdk-chemistry/skills/qdk-chemistry-coding/references/example-benzene-state-prep.md | Adds worked example reference. |
| copilot-plugins/qdk-chemistry/plugin.json | Adds plugin manifest. |
| copilot-plugins/qdk-chemistry/agents/reviewer.agent.md | Adds reviewer agent definition. |
| copilot-plugins/qdk-chemistry/agents/researcher.agent.md | Adds researcher agent definition. |
| copilot-plugins/qdk-chemistry/agents/reporter.agent.md | Adds reporter agent definition. |
| copilot-plugins/qdk-chemistry/agents/quantum-agent.agent.md | Adds orchestrator agent definition. |
| copilot-plugins/qdk-chemistry/agents/chemist.agent.md | Adds chemist agent definition. |
| copilot-plugins/qdk-chemistry/.mcp.json | Adds plugin MCP server config (requires runtime workspace binding). |
| .pre-commit-config.yaml | Adjusts markdown lint exclusions and interrogate threshold. |
| .pipelines/requirements.txt | Updates pipeline lockfile to include MCP stack and deps. |
| .pipelines/requirements.in | Adds MCP dependency to pipeline inputs. |
| .gitignore | Minor formatting change. |
| .github/plugin/marketplace.json | Adds Copilot plugin marketplace manifest. |
| .devcontainer/devcontainer.json | Forwards MCP port 8081 for devcontainer usage. |
Suppressed comments (2)
docs/source/user/agents.rst:124
- The manual-start examples use
qdk_chem_mcp, but the script added in this PR isqdkchemmcp(and the CLI help text elsewhere usesqc). This mismatch will confuse users and makes the docs copy/paste-fail unless an alias exists.
.github/plugin/marketplace.json:15 - The plugin entry version is also set to
2.0.0here, which conflicts with the plugin manifest version (2.1.0). Align the entry version so marketplace clients report the correct installed version.
"name": "qdk-chemistry",
"description": "Run QDK Chemistry workflows with specialized agents, skills, and MCP tools.",
"version": "2.0.0",
"source": "copilot-plugins/qdk-chemistry"
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 64 changed files in this pull request and generated 7 comments.
Suppressed comments (5)
Previously missed (3) — in code that hasn't changed since the last review.
.pre-commit-config.yaml:52
- Setting interrogate’s
--fail-underto 0 effectively disables docstring coverage enforcement while still running the hook. If the intent is to keep doc coverage meaningful, restore a non-trivial threshold (e.g., the previous 80) or remove the hook entirely.
args: [--fail-under=0, --verbose, --ignore-init-method]
python/src/qdk_chemistry/ui/io.py:23
- Path stripping uses
rsplit("/", ...), which is not portable on Windows paths ("\"). Useos.path.basenameto reliably drop directory components across platforms.
This issue also appears in the following locations of the same file:
- line 63
- line 86
python/src/qdk_chemistry/ui/validation.py:118
validate_projectis a decorator and always returns a callable wrapper, but its type signature/docstring currently suggest it may return a JSON string. The wrapper actually returns a plain error string on validation failure. Updating the annotation and docstring will prevent confusing API/docs for tool authors.
python/src/qdk_chemistry/ui/io.py:86
- Path stripping uses
rsplit("/", ...), which is not portable on Windows paths ("\"). Useos.path.basenameto reliably drop directory components across platforms.
python/src/qdk_chemistry/ui/io.py:63 - Path stripping uses
rsplit("/", ...), which is not portable on Windows paths ("\"). Useos.path.basenameto reliably drop directory components across platforms.
There was a problem hiding this comment.
🟡 Changes recommended
The docs build configuration and UI validation docs contain correctness/robustness issues that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
python/src/qdk_chemistry/ui/validation.py:226
validate_projectdocstring says the wrapper returns a JSON string on failure, but the implementation returns a plain text message (not JSON). This mismatch can confuse callers and documentation readers; either update the docstring or return structured JSON.
docs/source/conf.py:361normalize_public_export_modules()is executed at import-time inconf.pyand unconditionally importsqdk_chemistry.algorithms/qdk_chemistry.data. If those imports fail in a docs-only environment (e.g., native extensions not built), the entire docs build will crash. Consider guarding this with an ImportError handler so docs can still build (possibly with reduced API output).
# Sphinx 9 resolves type annotations while initializing extensions, before the
# ``builder-inited`` event. Normalize exports as part of loading this config so
# autodoc and sphinx-autodoc-typehints both see the same public object names.
normalize_public_export_modules()
- Files reviewed: 82/85 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new UI config initializes and creates filesystem directories at import time even when workspace binding is required, which can write outside the bound workspace before bind_workspace runs.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
python/src/qdk_chemistry/ui/config.py:77
- QDKMCPConfig creates scratch/projects/cache/jobs directories during init (import time). For plugin-launched MCP processes that require bind_workspace first (QDK_REQUIRE_WORKSPACE_BINDING=1), this performs filesystem writes before the workspace is bound, undermining the intended isolation.
- Files reviewed: 82/85 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The PR introduces a broad new MCP/plugin surface area plus remote-execution/registry behavior changes, and it includes at least one concrete robustness bug that should be fixed before approval.
Review details
- Files reviewed: 82/85 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The streamable-http Accept-header relaxer currently treats Accept: */* as already compliant, which contradicts its stated purpose and can still lead to 406 responses for wildcard-only clients.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
python/src/qdk_chemistry/ui/mcp.py:173
- In
_accept_relaxer_middleware, treatingAccept: */*as already satisfying bothapplication/jsonandtext/event-streamprevents the middleware from rewriting the header for wildcard-only clients. The docstring above states wildcard-only Accept headers are a known incompatibility that should be fixed by this middleware, so this logic currently won't help those clients and may still yield 406 responses.
- Files reviewed: 82/85 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The owner-scoped cache key uses a truncated SHA-256 prefix that can collide and should be strengthened to better preserve cache/job isolation semantics.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
python/src/qdk_chemistry/remote/proxy.py:94
_job_cache_key()truncates the SHA-256 digest to 16 hex chars (64 bits). Since this key is used to isolate jobs/results by owner, a longer digest would reduce the chance of collisions that could cause cross-owner cache/job confusion in shared caches.
python/src/qdk_chemistry/remote/worker.py:51- The owner-scoped
job_cache_keyfallback uses a SHA-256 digest truncated to 16 hex chars (64 bits). Because this key is used for owner isolation in caches, consider using a longer prefix to further reduce collision risk.
- Files reviewed: 82/85 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The change set is large and cross-cutting (new MCP surface area + cache/job semantics + registry/alias enforcement), so it warrants final human review to confirm no regressions in existing workflows.
Review details
- Files reviewed: 82/85 changed files
- Comments generated: 0 new
- Review effort level: Lite
David Williams-Young (wavefunction91)
left a comment
There was a problem hiding this comment.
Re-reviewed the amended head. The remaining qubit-output blocker is fixed and reproduced unmocked; the targeted nuclear/CLI regressions pass. No hard blocker remains.
No description provided.